New setting AllowUpdateOverwrite : defines how UPDATE should handle the case when a record was updated by trigger. by hvlad · Pull Request #8957 · FirebirdSQL/firebird

hvlad · 2026-03-26T09:04:11Z

After #8538 there was long private discussion with customer (big sponsor of Firebird).
The root of the issue is the way Firebird implements per-row triggers - it is far from SQL standard in regards of consistency.
There is well-known historical reasons for it and I don't want to repeat it here.
By my opinion, the best way to fix the issue would be to re-implement per-row triggers according to SQL standard - to fire before and after whole statement processing. But this is not quick nor easy task.
Finally this patch was implemented and customer uses it to prepare own migration to v5.
Now it is time to decide if it could be accepted into codebase.

…e case when a record was updated by trigger.

AlexPeshkoff · 2026-03-26T10:04:48Z

On 3/26/26 12:04, Vlad Khorsun wrote: After #8538 <#8538> there was long private discussion with customer (big sponsor of Firebird). The root of the issue is the way Firebird implements per-row triggers - it is far from SQL standard in regards of consistency. There is well-known historical reasons for it and I don't want to repeat it here. By my opinion, the best way to fix the issue would be to re-implement per-row triggers according to SQL standard - to fire before and after whole statement procesing. But this is not quick nor easy task. Finally this patch was implemented and customer uses it to prepare own migration to v5.

Vlad, am I missing something or this patch works only with system triggers that implement cascade FK?

hvlad · 2026-03-26T10:16:08Z

Vlad, am I missing something or this patch works only with system triggers that implement cascade FK?

The routine, I modified in the patch, was initially used to workaroud issue with self-referenced FK's

dyemanov

Honestly, I expected this PR to implement the same logic as seems to exist for FKs:

If field was not changed by user - pick up possible modification by trigger

instead of raising an error unconditionally.

However, AFAIU, we cannot detect this reliably at the VIO layer, because cannot distinguish between new.field explicitly assigned to the old value or copied from org_record. This would need accessing the ModifyNode's assignment list and checking for modified fields there. Although I'm not sure this is going to be as simple.

Did you consider this approach? Because the new mode reminds me the famous "table is mutating" error in Oracle, if raised even for non-conflicting changes.

dyemanov · 2026-04-03T10:17:43Z

builds/install/misc/firebird.conf

+#
+# Per-database configurable.
+#
+# Type: integer


Why integer and 0/1 instead of boolean and false/true? And rename to something like UpdateOverwriteStrictMode or RestrictUpdateOverwrite?

Initially I thought about 3rd value - to issue warning, not error. But it was not implemented as I found it unpractical, iirc.

As for the setting name - I can agree with anything reasonable ;) Lets decide if we need such changes.

Also, I going to add forgoten note that this setting is temporary and could be removed in future versions.

As for the setting name - I can agree with anything reasonable ;)

I suppose we need more opinions here. @mrotteveel , can you suggest any better naming for a boolean version of the switch?

How about "DisableTableMutation"?

Can you explain, please, what is bad with UpdateOverwriteMode ? Don't you like Mode ?

Exactly. Mode suggests more options than just true/false, or maybe two options but explicitly named -- like enum UpdateOverwriteMode = { Ignore , Restrict }. This is why I initially suggested StrictMode or alike, because it converts Mode into a boolean.

AllowUpdateOverwriteTrigger or simple AllowUpdateOverwrite ?

AllowUpdateOverwriteTrigger or simple AllowUpdateOverwrite ?

I assume you mean these in combination with an enum as Dmitry suggested. Then I'd suggest something like:

TriggerLostUpdateBehaviour = { Allow | Reject }

AllowUpdateOverwriteTrigger or simple AllowUpdateOverwrite ?

I assume you mean these in combination with an enum as Dmitry suggested.

I meant it should be a boolean. I should have state it explicitly, sorry.

Then I'd suggest something like:

TriggerLostUpdateBehaviour = { Allow | Reject }

No, please. "Lost update" is well known term from another area - transaction's isolation and concurrency.
And there is no "lost update" in classical meaning here.

AllowUpdateOverwriteTrigger or simple AllowUpdateOverwrite ?

Either AllowUpdateOverwrite[Trigger] (with default = true) or RejectUpdateOverwrite[Trigger] (with default = false) are fine with me.

Ok, I'll change it by AllowUpdateOverwriteTrigger with default = true.

src/jrd/vio.cpp

dyemanov · 2026-04-03T10:57:55Z

src/jrd/vio.cpp

+			const bool flag_cur = EVL_field(relation, cur_rpb->rpb_record, fld, &dsc_cur);
+
+			// Check if current record differs from old record
+			if ((flag_cur != flag_old) ||


If the field didn't exist in the old format but exists in the new format and gets some value updated by a trigger (e.g. because the new field is NOT NULL), an error will be raised. Is it intended?

If any field of current record was changed by trigger, that fired for another record - yes, this is error.
Despite of formats.

hvlad · 2026-04-03T12:46:14Z

Honestly, I expected this PR to implement the same logic as seems to exist for FKs:

If field was not changed by user - pick up possible modification by trigger

instead of raising an error unconditionally.

I consider this as wrong way because it let user triggers to implicitly change original UPDATE semantic.
Also, it is unpredictable and depends on internal code path and physical records order.
This is definitely not consistent behavior.

Case with self-referenced FK's is more predictable and consistent (unless same fields not changed by user trigger).

However, AFAIU, we cannot detect this reliably at the VIO layer, because cannot distinguish between new.field explicitly assigned to the old value or copied from org_record. This would need accessing the ModifyNode's assignment list and checking for modified fields there. Although I'm not sure this is going to be as simple.

Did you consider this approach?

No. By the reasons as explained above.

Because the new mode reminds me the famous "table is mutating" error in Oracle, if raised even for non-conflicting changes.

The more I think on this issue, the more I agree with Oracle - it should not be allowed. And, actually, I've implemented such check when works on the issue past year. But - not a surprise - it was too restrictive and customer does not accepted it. Finally, the setting UpdateOverwriteMode was introduced and agreed as compromise solution.

Ideally, we should reimplement the way per-row triggers works to provide consistensy and comply with SQL standard.

dyemanov · 2026-04-04T06:34:22Z

Well, provided that it's disabled by default, I can accept this patch. Did you try running the QA pass with UpdateOverwriteMode = 1?

hvlad · 2026-04-04T12:20:17Z

Did you try running the QA pass with UpdateOverwriteMode = 1?

Yes, but I don't remember the results, sorry. Will try once more time.

hvlad · 2026-04-04T21:44:59Z

Did you try running the QA pass with UpdateOverwriteMode = 1?

Yes, but I don't remember the results, sorry. Will try once more time.

Found no differences. Same number and list of failed and passed tests in two runs.

hvlad · 2026-04-06T12:55:01Z

Did you try running the QA pass with UpdateOverwriteMode = 1?

Yes, but I don't remember the results, sorry. Will try once more time.

Found no differences. Same number and list of failed and passed tests in two runs.

I'm sorry, missed one failed test: tests\bugs\core_3362_complex_test.py

It shows one more (forgotten) case when update could silently overwrite prior update - when second update is positioned on explicit (stable) cursor:

-- positioned, positioned
for select ... from table <table> as cursor c
  do begin
    update table set ... where current of c;  -- 1
    update table set ... where current of c;  -- 2
  end

-- or

-- searched, positioned
for select ... from table <table> as cursor c
  do begin
    update table set ... where id = c.id;  -- 1
    update table set ... where current of c;  -- 2
  end

Here: update (2) silently overwrites changes by update (1) and error is raised when AllowUpdateOverwrite = false

I consider this as correct behavior that should be properly documented.
Therefore I'll use 'AllowUpdateOverwrite' name for the setting (i.e. not mention trigger) and correct setting description and error message text.

Test case core_3362_complex_test should be fixed accordingly.

aafemt · 2026-04-06T12:58:44Z

I don't think that raising an error on two separate updates that operate on the same record is a correct behavior.

hvlad · 2026-04-06T13:08:37Z

I don't think that raising an error on two separate updates that operate on the same record is a correct behavior.

You missed the point - updates are not separate.

Specially for you:

insert into t vaues (1, '', '');
for select * from t as cursor c
do begin
  update t set a = 'a' where currect of c;
  update t set b = 'b' where currect of c;
end
select * from t;

Most users expects to see (1, 'a', 'b') here but actuall result is (1, '', 'b')

aafemt · 2026-04-06T13:11:39Z

Yes, this point I missed.

I would also expect (1, 'a', 'b'), so seeing (1, '', 'b') is a definite bug, but throwing an error is not better.

hvlad · 2026-04-06T13:19:48Z

Yes, this point I missed.

I would also expect (1, 'a', 'b'), so seeing (1, '', 'b') is a definite bug, but throwing an error is not better.

Not a bug but implementation detail. Maybe not so obvious.
Error allows users to detect such code and fix it to not produce such behavior.

Also, nobody forces you to set AllowUpdateOverwrite = false.

I'm not happy to introduce such settings, but see no better alternative. At least for the stable branch.

aafemt · 2026-04-06T13:29:59Z

Error allows users to detect such code and fix it to not produce such behavior.

I'm not happy to introduce such settings, but see no better alternative. At least for the stable branch.

How about my suggestion above? Result is the same - the error is thrown, but at least it is thrown consistently for all cases "two updates in one routine", "trigger then main request" and "main request then trigger". All these cases can be fixed by changing of user code.

hvlad · 2026-04-06T13:48:27Z

Error allows users to detect such code and fix it to not produce such behavior.

I'm not happy to introduce such settings, but see no better alternative. At least for the stable branch.

How about my suggestion above? Result is the same - the error is thrown, but at least it is thrown consistently for all cases "two updates in one routine", "trigger then main request" and "main request then trigger". All these cases can be fixed by changing of user code.

As far as I understand your suggestion (to reuse MERGE logic):
a) it can't be directly applied to the positioned updates,
b) it can't directly handle the case when UPDATE run not by the trigger itself but by deeper statement in the call tree,

Also, the patch I offer is in use by the customer for a few months already and got intensive testing.
I don't want to allow new risky code into stable branch, especially before point release.

aafemt · 2026-04-06T13:59:16Z

As far as I understand your suggestion (to reuse MERGE logic):

Don't pay so much attention to the comparison. From "MERGE logic" in the suggestion is only throwing of an error on double update of the same record inside of the same statement.

a) it can't be directly applied to the positioned updates,
b) it can't directly handle the case when UPDATE run not by the trigger itself but by deeper statement in the call tree,

Perhaps you read it wrong. The proposed loop through savepoints can detect double update on any (desirable) level. No matter how deep is current update, the update on any higher level is reliable detected. Positioned update is not different to any other update in regard of marking a record as "touched" in VerbAction::vct_records.

aafemt · 2026-04-06T14:49:22Z

Also, the patch I offer is in use by the customer for a few months already and got intensive testing.
I don't want to allow new risky code into stable branch, especially before point release.

BTW, you don't need to throw whole your work away, just change record modification detection from fields' comparison to looking at backlog.

hvlad · 2026-04-06T16:25:56Z

How your "loop through savepoints" will handle this case ?

execute block
as
begin
  insert into t1 (id, ...) values (1, ...);
  update t1 set ... where id = 1;
end

aafemt · 2026-04-06T16:32:17Z

How your "loop through savepoints" will handle this case ?

By throwing an error that "allows users to detect such code and fix it to not produce such behavior". And only in the case if they purposefully "set AllowUpdateOverwrite = false".

hvlad · 2026-04-06T16:34:44Z

How your "loop through savepoints" will handle this case ?

By throwing an error that "allows users to detect such code and fix it to not produce such behavior". And only in the case if they purposefully "set AllowUpdateOverwrite = false".

Wasted time, again. Don't write to my topics anymore.

aafemt · 2026-04-06T16:37:31Z

Do you care to explain how this example is different from core_3362_complex_test?

hvlad · 2026-04-06T16:41:35Z

Do you care to explain how this example is different from core_3362_complex_test?

Find the stable cursor and positioned update, if you know what these words means.

… (boolean).

…Mode New setting AllowUpdateOverwrite : defines how UPDATE should handle the case when a record was updated by trigger.

New setting UpdateOverwriteMode : defines how UPDATE should handle th…

97f703a

…e case when a record was updated by trigger.

hvlad self-assigned this Mar 26, 2026

dyemanov self-requested a review April 3, 2026 10:11

dyemanov reviewed Apr 3, 2026

View reviewed changes

Follow some suggestions by @dyemanov

fe0cc14

Setting UpdateOverwriteMode (integer) renamed to AllowUpdateOverwrite…

a7b3f39

… (boolean).

dyemanov approved these changes Apr 7, 2026

View reviewed changes

hvlad changed the title ~~New setting UpdateOverwriteMode : defines how UPDATE should handle the case when a record was updated by trigger.~~ New setting AllowUpdateOverwrite : defines how UPDATE should handle the case when a record was updated by trigger. Apr 7, 2026

hvlad merged commit 13f7cf8 into v5.0-release Apr 7, 2026
43 of 44 checks passed

hvlad added the type: new feature label Apr 7, 2026

hvlad added the fix-version: 5.0.4 label Apr 7, 2026

hvlad added a commit that referenced this pull request Apr 7, 2026

Front-ported pull request #8957 from FirebirdSQL/work/UpdateOverwrite…

6f2f653

…Mode New setting AllowUpdateOverwrite : defines how UPDATE should handle the case when a record was updated by trigger.

hvlad added the fix-version: 6.0 Alpha 1 label Apr 7, 2026

Uh oh!

Conversation

hvlad commented Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexPeshkoff commented Mar 26, 2026 via email

Uh oh!

hvlad commented Mar 26, 2026

Uh oh!

dyemanov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hvlad Apr 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hvlad commented Apr 3, 2026

Uh oh!

dyemanov commented Apr 4, 2026

Uh oh!

hvlad commented Apr 4, 2026

Uh oh!

hvlad commented Apr 4, 2026

Uh oh!

hvlad commented Apr 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aafemt commented Apr 6, 2026

Uh oh!

hvlad commented Apr 6, 2026

Uh oh!

aafemt commented Apr 6, 2026

Uh oh!

hvlad commented Apr 6, 2026

Uh oh!

aafemt commented Apr 6, 2026

Uh oh!

hvlad commented Apr 6, 2026

Uh oh!

aafemt commented Apr 6, 2026

Uh oh!

aafemt commented Apr 6, 2026

Uh oh!

hvlad commented Apr 6, 2026

Uh oh!

aafemt commented Apr 6, 2026

Uh oh!

hvlad commented Apr 6, 2026

Uh oh!

aafemt commented Apr 6, 2026

Uh oh!

hvlad commented Apr 6, 2026

Uh oh!

Uh oh!

Reviewers

hvlad commented Mar 26, 2026 •

edited

Loading

hvlad Apr 5, 2026 •

edited

Loading

hvlad commented Apr 6, 2026 •

edited

Loading